Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add Global Alumni in external course sync #3330

Merged
merged 17 commits into from
Dec 12, 2024
Merged

feat: add Global Alumni in external course sync #3330

merged 17 commits into from
Dec 12, 2024

Conversation

Anas12091101
Copy link
Contributor

@Anas12091101 Anas12091101 commented Dec 2, 2024 •
edited
Loading

What are the relevant tickets?

https://github.com/mitodl/hq/issues/6107

Description (What does it do?)

This PR refactors the external course sync code to make it more generic and adds Global Alumni as a vendor. Additionally, it introduces the sync_daily property in the Platform model, which is set to False by default. For platforms that have an external API and require daily synchronization, the sync_daily property should be set to True. This can be updated via the Django admin interface.

How can this be tested?

  • Run the migrations using ./manage.py migrate
  • Follow the testing steps in feat: ingest external course APIs #2998 to test the management command sync_external_course_runs
  • To test the task_sync_external_course_runs, go to platform model in django admin
  • Create "Emeritus" and "Global Alumni" platforms if not already exist.
  • Open django shell using ./manage.py shell
  • Run the following code: 
    In [1]: from courses.tasks import task_sync_external_course_runs
In [2]: task_sync_external_course_runs.delay()
Out[2]: <AsyncResult: d6521ec0-8ba7-4852-af18-e38febdae33e>
  • Monitor the logs of the celery task, the task should be successful and it should create or update the required data using the API response.

Additional Context

The Emeritus and Global Alumni APIs return data with the same fields, except for the CEU field, which is present in Emeritus but not in Global Alumni.

@Anas12091101 Anas12091101 marked this pull request as draft December 2, 2024 20:17
@odlbot odlbot temporarily deployed to xpro-ci-pr-3330 December 2, 2024 20:18 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-3330 December 2, 2024 20:20 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-3330 December 2, 2024 20:20 Inactive
@mitodl mitodl deleted a comment from gitguardian bot Dec 2, 2024
@pdpinch
Copy link
Member

pdpinch commented Dec 2, 2024

When I first read the PR title, I thought it was for Google Analytics instead of Global Alumni. I think it would be good to spell out Global Alumni in the PR title and in the commit message.

@Anas12091101 Anas12091101 changed the title feat: add GA in external course sync feat: add Global Alumni in external course sync Dec 3, 2024
@gitguardian GitGuardian
Copy link

gitguardian bot commented Dec 3, 2024 •
edited
Loading

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@odlbot odlbot temporarily deployed to xpro-ci-pr-3330 December 3, 2024 06:08 Inactive
@Anas12091101 Anas12091101 marked this pull request as ready for review December 4, 2024 16:45
asadali145
asadali145 reviewed Dec 9, 2024
View reviewed changes
courses/sync_external_courses/external_course_sync_api_test.py
@@ -110,7 +110,7 @@ def external_course_data_with_non_usd_price(external_course_data):
external_course_json = external_course_data.copy()
external_course_json["list_currency"] = "INR"
external_course_json["course_run_code"] = (
f"{external_course_data["course_run_code"].split("-")[0]}-INRC-98-10#1"
f"{external_course_data['course_run_code'].split('-')[0]}-INRC-98-10#1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use double quotes. This applies to all the changes in the last 2 commits.

@Anas12091101
Copy link
Contributor Author

@cachob, FYI, in this PR:

  • The api/emeritus_courses/ endpoint has been changed to api/external_courses/<str:vendor>/.

    • Example: api/external_courses/Emeritus.
    • For vendors with spaces in their names (e.g., Global Alumni), the endpoint can be accessed as either api/external_courses/Global Alumni or api/external_courses/Global_Alumni.

  • A new boolean field, sync_daily, has been added to the Platform model.

    • This field can be configured from the Django admin panel.
    • By default, it is set to False for all platforms. To enable daily syncing for an external vendor, you need to set it to True in the admin panel.
    • You can configure this field here: https://rc.xpro.mit.edu/admin/courses/platform/1/change/

As previously discussed, we will keep syncing off for Global Alumni until the data is fixed.

@Anas12091101 Anas12091101 merged commit d09c364 into master Dec 12, 2024
7 checks passed
@Anas12091101 Anas12091101 deleted the anas/add-GA branch December 12, 2024 09:06
@odlbot odlbot mentioned this pull request Dec 12, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asadali145 asadali145 asadali145 approved these changes

Assignees

@asadali145 asadali145

Labels
Waiting on Author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Anas12091101 @pdpinch @asadali145 @odlbot